fix b2sum, md5sum and hashsum regressions by sylvestre · Pull Request #8760 · uutils/coreutils

sylvestre · 2025-09-28T15:51:14Z

No description provided.

github-actions · 2025-09-28T16:12:33Z

GNU testsuite comparison:

GNU test failed: tests/cksum/md5sum. tests/cksum/md5sum is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/misc/stdbuf (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/tail/overlay-headers (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/cksum/b2sum is no longer failing!
Congrats! The gnu test tests/cksum/cksum-a is no longer failing!

sylvestre · 2025-09-28T16:38:51Z

    BitsRequiredForShake256,
+    #[error("--bits required for SHA2")]
+    BitsRequiredForSha2,
+    #[error("Invalid output size for SHA2 (expected 224, 256, 384, or 512), got {0}")]


i noticed that we aren't translating this file, i will fix it next

github-actions · 2025-09-28T17:22:28Z

GNU testsuite comparison:

GNU test failed: tests/cksum/md5sum. tests/cksum/md5sum is passing on 'main'. Maybe you have to rebase?
Skipping an intermittent issue tests/misc/stdbuf (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/misc/tee (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/cksum/b2sum is no longer failing!
Congrats! The gnu test tests/cksum/cksum-a is no longer failing!

codspeed-hq · 2025-09-28T17:36:49Z

CodSpeed Performance Report

Merging #8760 will not alter performance

_{Comparing sylvestre:cksum (06b8692) with main (52c71dc)}

Summary

✅ 41 untouched
⏩ 64 skipped¹

64 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

github-actions · 2025-09-28T18:19:24Z

GNU testsuite comparison:

Congrats! The gnu test tests/cksum/b2sum is no longer failing!
Congrats! The gnu test tests/cksum/cksum-a is no longer failing!

cakebaker · 2025-09-29T08:14:29Z

+            .arg("lorem_ipsum.txt")
+            .fails_with_code(1)
+            .no_stdout()
+            .stderr_contains("--bits required for SHA2");


The expected error message is incorrect as there is no --bits option.

Suggested change

.stderr_contains("--bits required for SHA2");

.stderr_contains("--length required for SHA2");

GNU cksum itself shows the following error:

cksum: --algorithm=sha2 requires specifying --length 224, 256, 384, or 512

cakebaker · 2025-09-29T08:29:16Z

+    }
+
+    #[test]
+    fn test_sha2_with_length_224() {


It might make sense to combine the test_sha2_with_length_xxx tests into a single test function that loops over the valid lengths.

Maybe we could leverage rstest here. It offers parametrized tests (the macro generates all the testcases independently). See https://github.com/la10736/rstest?tab=readme-ov-file#parametrize

cakebaker · 2025-09-29T08:52:44Z

+        // Should contain base64 characters and SHA256 label
+        assert!(result.contains("SHA256"));
+        // Base64 should end with = or == or contain +/
+        assert!(result.contains('=') || result.contains('+') || result.contains('/'));


I would use a single assert and check for the correct output. The second assert is currently true independent of whether the output is base64 because result.contains('=') matches the delimiter between hash name and hash. And the "or" conditions are not necessary because you know the expected output.

cakebaker · 2025-09-29T09:29:27Z

+    #[error("--bits required for SHA2")]
+    BitsRequiredForSha2,
+    #[error("Invalid output size for SHA2 (expected 224, 256, 384, or 512), got {0}")]
+    InvalidSha2Length(usize),


For SHA3 we use InvalidOutputSizeForSha3, and so for consistency reasons I would use a similar name:

Suggested change

InvalidSha2Length(usize),

InvalidOutputSizeForSha2(usize),

RenjiSann · 2025-09-29T10:23:05Z

+                    let error_key = format!("{}-error-invalid-length", utility_name);
+                    let max_key = format!("{}-error-max-digest-length", utility_name);
+                    show_error!(
+                        "{}",
+                        translate!(&error_key, "length" => DisplayQuotable::quote(length_str))
+                    );
+                    Err(io::Error::new(
+                        io::ErrorKind::InvalidInput,
+                        translate!(&max_key, "algorithm" => "BLAKE2b", "max_bits" => 512),
+                    )
+                    .into())


This part is almost a duplicate of the ones above, there might be a way to factorize it with closures ?

cakebaker · 2025-09-29T12:36:11Z


+/// Validate BLAKE2b length with Fluent error messages
+/// This function is used by utilities that need localized error messages
+pub fn validate_blake2b_length(length_str: &str, utility_name: &str) -> UResult<Option<usize>> {


I'm struggling with this function as it duplicates the code of validate_blake2b_length_str and calculate_blake2b_length. And thus it makes it easy to introduce sync issues in the future.

cakebaker · 2025-09-29T12:46:53Z

 pub const ALGORITHM_OPTIONS_SM3: &str = "sm3";
 pub const ALGORITHM_OPTIONS_SHAKE128: &str = "shake128";
 pub const ALGORITHM_OPTIONS_SHAKE256: &str = "shake256";
+pub const ALGORITHM_OPTIONS_SHA2: &str = "sha2";


A detail: I would move this line up to line 39, so that it is between the consts for SHA1 and SHA3.

cakebaker

Done :)

cakebaker · 2025-10-27T14:22:36Z

@sylvestre I guess this PR needs a rework as tests/cksum/cksum-a got fixed by #9035.

cakebaker · 2025-12-16T15:15:48Z

@sylvestre is this PR still relevant? In the meantime all failing cksum GNU tests have been fixed.

sylvestre · 2025-12-16T15:18:24Z

probably not :)
thanks

sylvestre commented Sep 28, 2025

View reviewed changes

sylvestre force-pushed the cksum branch from 976e107 to 90cdc34 Compare September 28, 2025 16:56

sylvestre added 2 commits September 28, 2025 19:02

b2sum: better validation of the args like in tests/cksum/b2sum

3d52a89

cksum: add support for sha2

06b8692

sylvestre force-pushed the cksum branch from 90cdc34 to 4a76833 Compare September 28, 2025 17:02

sylvestre force-pushed the cksum branch from 4a76833 to 06b8692 Compare September 28, 2025 17:59

sylvestre marked this pull request as ready for review September 29, 2025 07:29

sylvestre requested a review from cakebaker September 29, 2025 07:29

cakebaker reviewed Sep 29, 2025

View reviewed changes

RenjiSann reviewed Sep 29, 2025

View reviewed changes

cakebaker reviewed Sep 29, 2025

View reviewed changes

sylvestre closed this Dec 16, 2025

	.stderr_contains("--bits required for SHA2");
	.stderr_contains("--length required for SHA2");

Uh oh!

Conversation

sylvestre commented Sep 28, 2025

Uh oh!

github-actions Bot commented Sep 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 28, 2025

Uh oh!

codspeed-hq Bot commented Sep 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #8760 will not alter performance

Summary

Footnotes

Uh oh!

github-actions Bot commented Sep 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cakebaker left a comment

Choose a reason for hiding this comment

Uh oh!

cakebaker commented Oct 27, 2025

Uh oh!

cakebaker commented Dec 16, 2025

Uh oh!

sylvestre commented Dec 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codspeed-hq Bot commented Sep 28, 2025 •

edited

Loading